Conversation
WalkthroughChanges to Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: unknown linters: 'unusedfunc,unusedparams', run 'golangci-lint help linters' to see the list of supported linters Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/wshutil/wshrpc.go (1)
277-287: Consider logging unmarshal errors for debugging.The silent error handling on lines 283-285 might make it difficult to debug malformed event data. While events are fire-and-forget, logging errors would help identify issues without breaking the event flow.
Consider this change:
func (w *WshRpc) handleEventRecv(req *RpcMessage) { if req.Data == nil { return } var waveEvent wps.WaveEvent err := utilfn.ReUnmarshal(&waveEvent, req.Data) if err != nil { + log.Printf("[%s] failed to unmarshal wave event: %v\n", w.DebugName, err) return } w.EventListener.RecvEvent(&waveEvent) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
pkg/wshutil/wshrpc.go(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: esimkowitz
Repo: wavetermdev/waveterm PR: 1790
File: pkg/remote/fileshare/wshfs/wshfs.go:122-122
Timestamp: 2025-01-22T01:28:41.417Z
Learning: The RpcClient in pkg/remote/fileshare/wshfs/wshfs.go is initialized and handled downstream by either main-server or wshcmd-connserver, as documented in the package comment.
📚 Learning: 2025-01-22T01:28:41.417Z
Learnt from: esimkowitz
Repo: wavetermdev/waveterm PR: 1790
File: pkg/remote/fileshare/wshfs/wshfs.go:122-122
Timestamp: 2025-01-22T01:28:41.417Z
Learning: The RpcClient in pkg/remote/fileshare/wshfs/wshfs.go is initialized and handled downstream by either main-server or wshcmd-connserver, as documented in the package comment.
Applied to files:
pkg/wshutil/wshrpc.go
📚 Learning: 2025-01-22T22:27:25.739Z
Learnt from: esimkowitz
Repo: wavetermdev/waveterm PR: 1790
File: pkg/remote/connparse/connparse.go:76-82
Timestamp: 2025-01-22T22:27:25.739Z
Learning: The GetRpcContext() method in wshutil package is guaranteed to never return nil due to type constraints, making nil checks unnecessary.
Applied to files:
pkg/wshutil/wshrpc.go
🧬 Code graph analysis (1)
pkg/wshutil/wshrpc.go (4)
pkg/wps/wpstypes.go (1)
WaveEvent(25-31)pkg/util/utilfn/marshal.go (1)
ReUnmarshal(36-42)pkg/wshutil/wshevent.go (1)
EventListener(20-23)pkg/wshrpc/wshrpctypes.go (1)
Command_EventRecv(88-88)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
pkg/wshutil/wshrpc.go (3)
272-274: Good addition for observability.The pprof labeling by RPC command will enable better performance profiling and debugging of RPC handlers.
289-291: Clean refactor of event handling.Extracting EventRecv logic to a dedicated helper improves code organization and readability.
674-677: Critical fix: Resolves the memory leak.This change correctly ensures that response handlers are always removed from
ResponseHandlerMap, even when thedoneflag is already set. Previously, the early return on line 678 (whendone.Load()was true) would skip unregistering, causing handlers to accumulate indefinitely in the map.The fix is thread-safe:
unregisterResponseHandleruses a lock, and multiple calls are idempotent (delete from map is safe even if key doesn't exist).
1 participant
always remove from reqmap in Finalize even if done flag is already set.